Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Fix exceptions and dynamic casts across dll boundaries #2828

Closed
wants to merge 4 commits into from

Conversation

John-Colvin
Copy link
Contributor

@John-Colvin John-Colvin commented Oct 11, 2019

By making the checks for whether something is a base class use an implementation like TypeInfo_Class.opEquals (Can't actually use == because that does a dynamic cast, which calls _d_isbaseof2, which means infinite recursion).

Relying on name comparison only seems weak, but ultimately the name is the id, just like names of symbols (on linux then the loader will implement ODR by eliminating duplicates based on names anyway, right?).

Effectively this is a reboot of #92

https://issues.dlang.org/show_bug.cgi?id=7020

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @John-Colvin! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2828"

@adamdruppe
Copy link
Contributor

any idea why it is failing on the win with lld-link? t seems kinda arbitrary

@wilzbach
Copy link
Member

wilzbach commented Nov 1, 2019

This could be related/help: dlang/dmd#10525

@adamdruppe
Copy link
Contributor

@John-Colvin so the failure on win64 doesn't seem related to the other thing but have you confirmed this works on win32? or is that talk about the throw function and catch tables needing to be merged mean there's still a lot to do?

@rmanthorpe
Copy link
Contributor

rmanthorpe commented Feb 13, 2020

Using ClassInfo.name isn't good enough for the same reason as https://issues.dlang.org/show_bug.cgi?id=20579

class A(T) {}
template B(T) {
    struct C {}
}

void main() {
    assert(A!(B!int.C).classinfo.name != A!(B!string.C).classinfo.name); // assertion failure!
}

I think the only way to fix this properly is to put the mangle of a type into ClassInfo and compare that. If you're going to do that, I suggest using that to fix TypeInfo.opEquals while we're at it. I think we would just need the mangle in TypeInfo_Struct, TypeInfo_Class and TypeInfo_Enum + this change then both casting and TypeInfo.opEquals will work.

@adamdruppe
Copy link
Contributor

alas mangleof isn't available for subclasses... that means we need a dmd change to make the information available. and mangles can be absurdly large. idk.

@rmanthorpe
Copy link
Contributor

Or some other unique name, but it must be unique that the problem. Something like std.traits.fullyQualifiedName looks like it does the trick. There's a reason they get long - they have to be unique.

You can mitigate the cost of doing this by versioning it out on non-Windows platforms and with a little effort you can work out if you're in a different DLL and only do the string comparison in those cases.

@adamdruppe
Copy link
Contributor

fullyQualfiedName i think also doesn't work without virtuals

but i did just remember we have RTInfo so it doesn't need a dmd change per se; it can be done at CT with rtinfo in object.d. not a bad idea, might work at least as a hack.

@rmanthorpe
Copy link
Contributor

How does RTInfo help?

@adamdruppe
Copy link
Contributor

adamdruppe commented Feb 13, 2020 via email

@rmanthorpe
Copy link
Contributor

Ah that's the badger!

@kinke
Copy link
Contributor

kinke commented Aug 1, 2021

TypeInfo names of classes, structs and interfaces are unique now after #3527.

@dlang-bot dlang-bot removed the stalled label Aug 1, 2021
{
if (a is b)
return true;
return (a && b) && a.info.name == b.info.name;
Copy link
Contributor

@kinke kinke Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be sensible to restrict the name check to version (Shared), i.e., a shared druntime (.{so,dll}). I think it's reasonable to expect a process with multiple D binaries to share a single druntime; that's not an option with DMD on Windows, I know (so could be enabled for version (DigitalMars) version (Windows) in general).

It just wouldn't slow down dynamic casting when sticking with a static druntime.

@kinke
Copy link
Contributor

kinke commented Aug 20, 2021

This has landed in #3543, thx John.

@kinke kinke closed this Aug 20, 2021
@John-Colvin
Copy link
Contributor Author

This has landed in #3543, thx John.

Thanks for sorting this out, sorry for abandoning it for so long

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants